Add additional contextual information to invalid-style-prop warning by niole · Pull Request #4482 · facebook/react

niole · 2015-07-24T22:39:24Z

inline style prop error thrown for JSX React elements now names React class that erroring element is a child of. Fixes #4168

…that erroring element is a child of.

facebook-github-bot · 2015-07-24T22:41:13Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

niole · 2015-07-24T22:48:25Z

This works in a live setting when inside of a working app.

facebook-github-bot · 2015-07-24T22:51:55Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

jimfb · 2015-07-25T06:02:13Z

It's the style prop of the owner component? This doesn't look right to me. I don't think it's the "style prop of the owner"

I read the issue as being it would be helpful if the error message
contained the name of the react class that has the render method where the
style prop has bad syntax, which is what that piece of code returns
On Jul 24, 2015 11:03 PM, "Jim" notifications@github.com wrote:

In src/renderers/dom/shared/ReactDOMComponent.js
#4482 (comment):

@@ -263,7 +263,7 @@ function assertValidProps(component, props) {
}
invariant(
props.style == null || typeof props.style === 'object',

'The style prop expects a mapping from style properties to values, ' +

'The style prop of '+component._currentElement._owner._instance.proto.constructor.displayName + ' expects a mapping from style properties to values, ' +

It's the style prop of the owner component? This doesn't look right to me.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/react/pull/4482/files#r35480276.

Woops must have been sleepy when I submitted that. I would change it to say
'check the render method of '+code_snippet

I read the issue as being it would be helpful if the error message
contained the name of the react class that has the render method where the
style prop has bad syntax, which is what that piece of code returns
On Jul 24, 2015 11:03 PM, "Jim" notifications@github.com wrote:

In src/renderers/dom/shared/ReactDOMComponent.js
#4482 (comment):

@@ -263,7 +263,7 @@ function assertValidProps(component, props) {
}
invariant(
props.style == null || typeof props.style === 'object',

'The style prop expects a mapping from style properties to values, ' +

'The style prop of '+component._currentElement._owner._instance.proto.constructor.displayName + ' expects a mapping from style properties to values, ' +

It's the style prop of the owner component? This doesn't look right to me.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/react/pull/4482/files#r35480276.

jimfb · 2015-07-27T18:05:48Z

You need to have an if-check, because there is no guarantee that _owner isn't null (element could have been created outside of a render method), in which case this will throw.

…correct inline styling

zpao · 2015-07-29T00:57:08Z

Add %s into the string here, then pass getDecl..() as a 3rd argument.

niole · 2015-07-31T17:42:08Z

Should I add this prefix to the test and the actual code?
On Jul 31, 2015 10:41 AM, "Niole Nelson" niolenelson@gmail.com wrote:

Cool. I'm on it.
On Jul 31, 2015 10:36 AM, "Paul O’Shannessy" notifications@github.com
wrote:

And yes, you're missing the 'Invariant Violation:' prefix.

—
Reply to this email directly or view it on GitHub
#4482 (comment).

jimfb · 2015-07-31T17:43:40Z

@niole No, just the test. The issue is that the actual code adds it automatically when you invoke invariant but the test does not.

zpao · 2015-07-31T17:53:09Z

You still have a leading space inside the text which doesn't belong. But do we really need this new sentence? I don't think it really adds anything and with the addendum we'll say to check the render method of a component.

zpao · 2015-07-31T17:54:50Z

Please run your tests locally before pushing. You can iterate much more quickly that way instead of waiting for travis to tell you there are failures.

jimfb · 2015-07-31T17:57:24Z

You can run your tests using npm test.

If you read somewhere that you should use npm run jest, be aware that has been removed in favor of npm test.

niole · 2015-07-31T18:21:25Z

What's up with this eslint-plugin-react-internal and how do I get it?
On Jul 31, 2015 10:58 AM, "Jim" notifications@github.com wrote:

You can run your tests using npm test.

If you read somewhere that you should use npm run jest, be aware that has
been removed in favor of npm test.

—
Reply to this email directly or view it on GitHub
#4482 (comment).

zpao · 2015-07-31T22:35:50Z

npm install should do it. You may need to install npm >= 2.0 though for that to work. If you're on npm < 2 you might be able to do npm install ./eslint-rules to pick it up.

niole · 2015-08-19T20:50:48Z

@zpao @jimfb how does it look.

zpao · 2015-08-19T20:54:24Z

Please just leave the beginning of this message as it was. We really just need the addendum.

zpao · 2015-08-19T20:56:00Z

Sorry for the delay, GitHub doesn't send notifications when there are new / changed commits added.

…riginal

niole · 2015-08-20T01:53:57Z

@zpao @jimfb how does it look? I'm also wondering about how to run tests locally. I upgraded npm and everything's installing.

I then run npm test and get Cannot find module 'fbjs/scripts/babel/rewrite-requires'.
babel, which 'couldn't be found', and a lot of failed tests
I then upgraded jest to 5.x.
Now I have this jsdom 4.x onward only works on io.js, not Node.js™, and what seems like less failed tests

should I use io.js?

niole · 2015-08-27T23:26:15Z

@zpao @jimfb Does this need work?

Add additional contextual information to invalid-style-prop warning

zpao · 2015-08-28T01:28:10Z

I squashed, cleaned up the unrelated changes, and pushed (your commit: 70c07c6, merge: bdc7ce9). Thanks for sticking with it!

niole · 2015-08-28T17:15:31Z

@zpao Thanks!

On Thu, Aug 27, 2015 at 6:29 PM, Paul O’Shannessy notifications@github.com
wrote:

I squashed, cleaned up the unrelated changes, and pushed (your commit:
70c07c6
70c07c6,
merge: bdc7ce9
bdc7ce9).
Thanks for sticking with it!

—
Reply to this email directly or view it on GitHub
#4482 (comment).

resolves issue facebook#4168, style prop error now names React class …

c0d7899

…that erroring element is a child of.

facebook-github-bot added the GH Review: review-needed label Jul 24, 2015

facebook-github-bot added the CLA Signed label Jul 24, 2015

jimfb reviewed Jul 25, 2015
View reviewed changes

jimfb added GH Review: needs-revision and removed GH Review: review-needed labels Jul 25, 2015

fixed error message

f6aa167

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jul 25, 2015

fix 100cw

8664478

jimfb changed the title ~~resolves issue #4168,~~ Add additional contextual information to invalid-style-prop warning Jul 27, 2015

jimfb reviewed Jul 27, 2015
View reviewed changes

zpao added GH Review: needs-revision and removed GH Review: review-needed labels Jul 27, 2015

use getDeclarationErrorAddendum() to identify the react class with in…

acfdec4

…correct inline styling

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jul 28, 2015

zpao reviewed Jul 29, 2015
View reviewed changes

zpao added GH Review: needs-revision and removed GH Review: review-needed labels Jul 29, 2015

use string interpolation

3dc3631

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jul 29, 2015

niole added 3 commits July 29, 2015 18:07

fix tests

8837602

fix tests 2

ebf2581

fix tests 3

e159f44

fix string interpolation, add 'Invariant Violation:', change tests

de38170

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jul 31, 2015

remove 'Invariant Violation:' prefix from invariant

d6cafe2

zpao reviewed Jul 31, 2015
View reviewed changes

remove spaces

2cd8195

improve inline style error message

d87b1b3

zpao reviewed Aug 19, 2015
View reviewed changes

add test confirming addendums existence, revert style prop error to o…

902a53c

…riginal

niole force-pushed the issue4168 branch from a413853 to 902a53c Compare August 20, 2015 00:35

zpao added a commit that referenced this pull request Aug 28, 2015

Merge pull request #4482 from niole/issue4168

bdc7ce9

Add additional contextual information to invalid-style-prop warning

zpao closed this Aug 28, 2015

zpao added GH Review: accepted and removed GH Review: review-needed labels Aug 28, 2015

zpao mentioned this pull request Aug 28, 2015

added owner in invalid style invariant error #4176

Closed

Conversation

niole commented Jul 24, 2015

Uh oh!

facebook-github-bot commented Jul 24, 2015

Uh oh!

niole commented Jul 24, 2015

Uh oh!

facebook-github-bot commented Jul 24, 2015

Uh oh!

jimfb Jul 25, 2015

Choose a reason for hiding this comment

Uh oh!

niole Jul 25, 2015

Choose a reason for hiding this comment

Uh oh!

niole Jul 25, 2015

Choose a reason for hiding this comment

Uh oh!

jimfb Jul 27, 2015

Choose a reason for hiding this comment

Uh oh!

zpao Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

niole commented Jul 31, 2015

Uh oh!

jimfb commented Jul 31, 2015

Uh oh!

zpao Jul 31, 2015

Choose a reason for hiding this comment

Uh oh!

zpao commented Jul 31, 2015

Uh oh!

jimfb commented Jul 31, 2015

Uh oh!

niole commented Jul 31, 2015

Uh oh!

zpao commented Jul 31, 2015

Uh oh!

niole commented Aug 19, 2015

Uh oh!

zpao Aug 19, 2015

Choose a reason for hiding this comment

Uh oh!

zpao commented Aug 19, 2015

Uh oh!

niole commented Aug 20, 2015

Uh oh!

niole commented Aug 27, 2015

Uh oh!

zpao commented Aug 28, 2015

Uh oh!

niole commented Aug 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants